Skip to content

Conversation

@stewartadam
Copy link
Member

@stewartadam stewartadam commented Dec 3, 2025

Pull Request

Description

Captures learnings from recent customer projects to enhance the build speed and launch reliability of the dev container.

  • removes unused Python feature from container, which was causing compilation of Python on build
  • add volume mounts for node_modules and user config for better I/O performance
  • normalize workspace mount to /workspace for consistency across environments
  • add CA certificate injection for corporate TLS inspection (ZScaler, etc.)
  • add git config include parsing workaround for devcontainer identity issues
  • add containerEnv vars for pip, nodejs, and uv SSL certificate bundles

🚀 - Generated by Copilot

Related Issue(s)

Type of Change

Select all that apply:

Code & Documentation:

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Breaking change (fix or feature causing existing functionality to change)
  • Documentation update

Infrastructure & Configuration:

  • GitHub Actions workflow
  • Linting configuration (markdown, PowerShell, etc.)
  • Security configuration
  • DevContainer configuration
  • Dependency update

AI Artifacts:

  • Reviewed contribution with prompt-builder chatmode and addressed all feedback
  • Copilot instructions (.github/instructions/*.instructions.md)
  • Copilot prompt (.github/prompts/*.prompt.md)
  • Copilot chatmode (.github/chatmodes/*.chatmode.md)

Note for AI Artifact Contributors:

  • Chatmodes: Research, indexing/referencing other project (using standard VS Code GitHub Copilot/MCP tools), planning, and general implementation chatmodes likely already exist. Review .github/chatmodes/ before creating new ones.
  • Model Versions: Only contributions targeting the latest Anthropic and OpenAI models will be accepted. Older model versions (e.g., GPT-3.5, Claude 3) will be rejected.
  • See Chatmodes Not Accepted and Model Version Requirements.

Other:

  • Script/automation (.ps1, .sh, .py)
  • Other (please describe):

Testing

Checklist

Required Checks

  • Documentation is updated (if applicable)
  • Files follow existing naming conventions
  • Changes are backwards compatible (if applicable)

Required Automated Checks

The following validation commands must pass before merging:

  • Markdown linting: npm run lint:md
  • Spell checking: npm run spell-check
  • Frontmatter validation: npm run lint:frontmatter
  • Link validation: npm run lint:md-links
  • PowerShell analysis: npm run lint:ps

Security Considerations

  • This PR does not contain any sensitive or NDA information
  • Any new dependencies have been reviewed for security issues
  • Security-related scripts follow the principle of least privilege

Additional Notes

Copilot AI review requested due to automatic review settings December 3, 2025 01:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the devcontainer configuration to improve build performance and reliability, particularly for developers working behind corporate proxies with TLS inspection. The changes remove unused features (Python, Azure CLI), add volume mounts for better I/O performance, normalize the workspace path, and add support for corporate CA certificate injection and git identity configuration.

Key changes include:

  • Removal of unused Python and Azure CLI features to speed up container builds
  • Addition of volume mounts for node_modules and user config to improve performance
  • Implementation of corporate CA certificate injection for TLS inspection compatibility
  • Git config workaround to properly handle conditional includes in devcontainers

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
.gitignore Adds gitconfig export files used by devcontainer git identity workaround
.gitattributes Defines line ending rules for Windows and Linux scripts (has duplicate entry)
.dockerignore Excludes .git/ and node_modules/ from Docker build context
.devcontainer/scripts/post-create.sh Adds volume ownership fixes, npm install, and CA cert injection logic
.devcontainer/scripts/post-attach.sh New script implementing git identity configuration workaround
.devcontainer/devcontainer.json Configures workspace mount, volumes, environment variables, lifecycle commands (has JSON syntax error)
.devcontainer/README.md Updates documentation to reflect removed features and add TLS troubleshooting
Comments suppressed due to low confidence (1)

.devcontainer/scripts/post-create.sh:44

  • Security concern: The CA certificate injection feature copies all .crt files from .devcontainer/ to the system trust store without validation. Consider adding validation to:
  1. Verify the files are valid PEM-formatted certificates before copying
  2. Add a warning message when certificates are being added
  3. Document the security implications in comments

Example:

if compgen -G ".devcontainer/*.crt" > /dev/null; then
  echo "WARNING: Adding custom CA certificates to system trust store"
  # Validate certs before copying
  for cert in .devcontainer/*.crt; do
    if openssl x509 -in "$cert" -noout 2>/dev/null; then
      sudo cp "$cert" /usr/local/share/ca-certificates/
    else
      echo "ERROR: Invalid certificate file: $cert"
      exit 1
    fi
  done
  sudo update-ca-certificates
fi
  if compgen -G ".devcontainer/*.crt" > /dev/null; then
    sudo cp .devcontainer/*.crt /usr/local/share/ca-certificates/
    sudo update-ca-certificates
  fi
  echo "Container's system CA certificates updated successfully"

Comment on lines +37 to +44
# Adds a root CA to the system certificate store. Useful if developer machines
# have MITM TLS inspection happening, e.g. with ZScaler.
echo "Updating container system CA certificates..."
if compgen -G ".devcontainer/*.crt" > /dev/null; then
sudo cp .devcontainer/*.crt /usr/local/share/ca-certificates/
sudo update-ca-certificates
fi
echo "Container's system CA certificates updated successfully"
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update_ca_certs function trusts any .crt file present in .devcontainer/ by copying it into the system CA store. This enables an attacker to commit a malicious root CA to the repo, causing the container to trust attacker-controlled TLS endpoints (e.g., for npm, pip, git), enabling MITM and credential/token theft. Restrict trusted CAs to a pre-approved, immutable source (e.g., mount a secure host-managed CA volume or require explicit opt-in via a verified checksum), and validate certificates before installation (e.g., whitelist filenames and verify SHA256).

Suggested change
# Adds a root CA to the system certificate store. Useful if developer machines
# have MITM TLS inspection happening, e.g. with ZScaler.
echo "Updating container system CA certificates..."
if compgen -G ".devcontainer/*.crt" > /dev/null; then
sudo cp .devcontainer/*.crt /usr/local/share/ca-certificates/
sudo update-ca-certificates
fi
echo "Container's system CA certificates updated successfully"
# Adds a root CA to the system certificate store, but only if it matches a pre-approved whitelist.
# This prevents malicious CAs from being trusted.
echo "Updating container system CA certificates..."
# Define whitelist: filename:sha256sum (add entries as needed)
declare -A CA_CERT_WHITELIST=(
# Example entry (replace with your actual cert and hash):
# ["my-org-root.crt"]="0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
)
local found_valid=0
for certfile in .devcontainer/*.crt; do
if [[ ! -f "${certfile}" ]]; then
continue
fi
certname="$(basename "${certfile}")"
expected_hash="${CA_CERT_WHITELIST[${certname}]:-}"
if [[ -z "${expected_hash}" ]]; then
echo "WARNING: Skipping unapproved CA certificate: ${certname}"
continue
fi
actual_hash="$(sha256sum "${certfile}" | awk '{print $1}')"
if [[ "${actual_hash}" != "${expected_hash}" ]]; then
echo "WARNING: Hash mismatch for ${certname}, skipping."
continue
fi
echo "Installing approved CA certificate: ${certname}"
sudo cp "${certfile}" /usr/local/share/ca-certificates/
found_valid=1
done
if (( found_valid )); then
sudo update-ca-certificates
echo "Container's system CA certificates updated successfully"
else
echo "No valid CA certificates installed."
fi

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WilliamBerryiii thoughts? I'm inclined to consider this a non-risk, if an attacker has access to the repo they can do far worse damage by updating any of the initialization scripts for the dev container.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love perspective from @agreaves-ms on this ... I'm on the fence here (mainly because I'm on windows and mucking with the cert store can be a PITA if things go sideways).

Copilot AI review requested due to automatic review settings December 3, 2025 01:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

.devcontainer/scripts/post-create.sh:42

  • Consider adding validation to check if the .crt files are valid PEM-formatted certificates before copying them to the system certificate store. Blindly copying arbitrary files with .crt extension could introduce security risks if the files are malformed or malicious. Add a basic validation check using openssl x509 -in "$file" -noout before copying.
  if compgen -G ".devcontainer/*.crt" > /dev/null; then
    sudo cp .devcontainer/*.crt /usr/local/share/ca-certificates/
    sudo update-ca-certificates

Copilot AI review requested due to automatic review settings December 3, 2025 01:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 9 comments.

Copilot AI review requested due to automatic review settings December 3, 2025 20:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings December 3, 2025 20:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.

Comment on lines +9 to +13
for conf in .gitconfig.global .gitconfig.local; do
if [[ -f "$conf" ]]; then
echo "*** Parsing ${conf##.gitconfig.} Git configuration export"
while IFS='=' read -r key value; do
local key value
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The local keyword should be declared before the variable assignment in the while read loop. Move the local key value declaration outside and before the loop to avoid redeclaring on each iteration.

Suggested fix:

copy_user_gitconfig() {
  local key value
  for conf in .gitconfig.global .gitconfig.local; do
    if [[ -f "$conf" ]]; then
      echo "*** Parsing ${conf##.gitconfig.} Git configuration export"
      while IFS='=' read -r key value; do
        case "$key" in
        user.name | user.email | user.signingkey | commit.gpgsign)
          echo "Set Git config ${key}=${value}"
          git config --global "$key" "$value"
          ;;
        esac
      done < "$conf"
      rm -f "${conf}"
    fi
  done
}
Suggested change
for conf in .gitconfig.global .gitconfig.local; do
if [[ -f "$conf" ]]; then
echo "*** Parsing ${conf##.gitconfig.} Git configuration export"
while IFS='=' read -r key value; do
local key value
local key value
for conf in .gitconfig.global .gitconfig.local; do
if [[ -f "$conf" ]]; then
echo "*** Parsing ${conf##.gitconfig.} Git configuration export"
while IFS='=' read -r key value; do

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants